Warn about <2024.1 entities spec and update tests #1272
Conversation
I do think that we'll want to translate it. I think we translate other workflow warnings. In general, we want to translate most warnings or errors that a user could trigger by user error. (There's getodk/central#489 to do more of that for errors.) |
| @@ -134,6 +134,7 @@ const createNew = (partial, project) => async ({ Actees, Datasets, FormAttachmen | |||
|
|
|||
| // Check for xmlFormId collisions with previously deleted forms | |||
| await Forms.checkDeletedForms(partial.xmlFormId, project.id); | |||
| await Forms.checkDatasetWarnings(parsedDataset); | |||
There was a problem hiding this comment.
I see that the lines above and below this one use await, but if Forms.checkDatasetWarnings() doesn't need to be async and return a promise, I think it'd be nice to remove the await. I think code is often clearer when things can be done sync.
There was a problem hiding this comment.
I'm stuck on how to not make this return a promise and not await it. It doesn't strictly need to be this way but it fits with checkMeta, checkDeletedForms, and checkStructuralChange in checking something and modifying the context on the container. (checkDeletedForms makes a DB call though so it does need to be this way.)
There was a problem hiding this comment.
I'm stuck on how to not make this return a promise and not await it.
In checkDatasetWarnings(), you can remove async, then replace return resolve(); with return;. After that, you can remove await here.
It doesn't strictly need to be this way but it fits with
checkMeta,checkDeletedForms, andcheckStructuralChangein checking something and modifying thecontexton the container. (checkDeletedFormsmakes a DB call though so it does need to be this way.)
checkDeletedForms() definitely needs to be async, but I don't think the other two do. checkMeta() calls reject(), but it could just throw. I think it's a little confusing that these functions are async, and I'd love to refactor them sometime. I don't think that time is now though! But it'd be great not to add another instance of this pattern.
There was a problem hiding this comment.
Following up something we talked about in review: changing this function to not return a promise leads to this error:
TypeError: Cannot read properties of undefined (reading 'pipe')
at module.<computed> [as checkDatasetWarnings] (/Users/ktuite/Desktop/code/odk/central-backend/lib/model/container.js:31:31)
at /Users/ktuite/Desktop/code/odk/central-backend/lib/model/query/forms.js:137:9
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async module.exports (/Users/ktuite/Desktop/code/odk/central-backend/test/integration/fixtures/02-forms.js:17:18)
at async Object.transaction (/Users/ktuite/Desktop/code/odk/central-backend/node_modules/slonik/dist/src/connectionMethods/transaction.js:22:24)
at async Object.createConnection (/Users/ktuite/Desktop/code/odk/central-backend/node_modules/slonik/dist/src/factories/createConnection.js:97:18) 1) "before all" hook in "{root}"
at these lines in the container. I could change these lines to use ?. but I'll leave it as is for now.
ktuite
force-pushed
the
ktuite/warnOldEntitySpec
branch
from
4014ab6
to
7c2173d
Compare
| </h:html>`, | ||
|
|
||
| // Copy of the above form with the original entities-version spec | ||
| updateEntity2023: `<?xml version="1.0"?> |
There was a problem hiding this comment.
I kind of think it'd be nice not to duplicate the XML if we don't have to. What if we did something like:
testData.forms.updateEntity2023 = testData.forms.updateEntity.replace('2024.1.0', '2023.1.0');No strong feelings if you prefer to keep it the way it is.
Closes getodk/central#730
I updated the versions in
testData.forms.simpleEntityandtestData.forms.updateEntityso the tests didn't have to change, but for any test that expected the older version, I also made test formstestData.forms.simpleEntity2022andtestData.forms.updateEntity2023and used those (along with?ignoreWarnings=true) where appropriate.Frontend PR: getodk/central-frontend#1059
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass OR confirm CircleCI build passes